revert(toolbox-core): Reverts "fix(toolbox-core): Prevent ToolboxClient from closing externally managed client sessions"
#271
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anubhav756
force-pushed
the
revert-260-anubhav-manage-session
branch
from
356c555 to
3ccdfad
Compare
anubhav756
changed the title
ToolboxClient from closing externally managed client sessions"ToolboxClient from closing externally managed client sessions"
…ng externally ma…" This reverts commit 7520adf.
anubhav756
force-pushed
the
revert-260-anubhav-manage-session
branch
from
3ccdfad to
46e1334
Compare
|
Why are we reverting these changes? |
anubhav756
changed the title
ToolboxClient from closing externally managed client sessions"ToolboxClient from closing externally managed client sessions"
kurtisvg
approved these changes
Jun 4, 2025
Added the details in the description. Let me know in case you need more context. 🙂 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reverts the
__del__method that was added toToolboxClientandToolboxSyncClient(#260) as a safety-net (context) to close the underlying client session.Reasoning for Reversion
The
__del__method was intended to prevent resource leaks by closing sessions that users forgot to close manually. However, it introduced a critical bug in production scenarios.The issue arises when the
ToolboxClientorToolboxSyncClientinstance, which manages the session, goes out of scope, while the tools it loaded remain in use. This triggers the__del__method on the client, prematurely closing the session and causing any ongoing operations by the tools to fail.This problem affects:
ToolboxClient: When it manages its own session.ToolboxSyncClient: In all use cases, as it always manages its own session in a background thread.Given that the documented methods for session management (
async withandclient.close()) are sufficient, this safety-net was causing more harm than good. Removing it restores the expected behavior and prevents these unintended session closures.